Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Faraday connection #212

Open
wants to merge 17 commits into
base: master
Choose a base branch
from
Open

Faraday connection #212

wants to merge 17 commits into from

Conversation

hexgnu
Copy link
Owner

@hexgnu hexgnu commented Feb 3, 2014

What

The internals of the linkedin gem are in need of some refactoring. This changes the internals to rely on Faraday and FaradayMiddleware to reduce some of the cruft.

The tests pass but I'm weary as to whether this is good enough yet or not.

One thing that I added was the ability to add middleware to linkedin so that for instance you could do something like this

LinkedIn.middleware << FaradayMiddleware::Caching

@hundredwatt @cmhobbs would love some second eyes on this.

@hundredwatt
Copy link
Collaborator

@hexgnu 👏

I'll review this later tonight

@hundredwatt
Copy link
Collaborator

@hexgnu I removed the fail-fast option we set in .rspec (aba9e3a)

The specs were failing for me while using MultiJSON 1.0.x. I think we'll need to raise that dependency before releasing this.

Were you thinking of releasing this as part of a 0.5 release?

Other than that, I ran my new specs from #211 against this. They passed when I ran only the new people_spec.rb file, but when I ran the whole suite they failed. I need to track down where the global state failed.

@hexgnu
Copy link
Owner Author

hexgnu commented Feb 4, 2014

yea I think releasing this as a 0.5.x would be good. And once we get around to introducing OAuth2 we can bump it to 0.6.

@hundredwatt
Copy link
Collaborator

Looks like the MultiJSON interface change happend at 1.3.0: intridea/multi_json@e90fd6c, I'll update the gemspec accordingly

@ismell
Copy link
Contributor

ismell commented Mar 25, 2014

Looks good!

@hosh
Copy link

hosh commented Oct 18, 2014

What's the status on this? Has this been merged yet?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants